Skip to content

test/pytest: a guard in a teardown cannot fail the test it guards (#432) - #931

Merged
jdatcmd merged 1 commit into
commandprompt:mainfrom
OffgridwithJD:audit/432-a-guard-must-fail-the-test
Sep 10, 2026
Merged

test/pytest: a guard in a teardown cannot fail the test it guards (#432)#931
jdatcmd merged 1 commit into
commandprompt:mainfrom
OffgridwithJD:audit/432-a-guard-must-fail-the-test

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

The same AssertionError, raised from two places:

raised from the run reports
a pytest_runtest_call wrapper 1 failed
a fixture teardown 1 passed, 1 error

By the time a teardown runs, pytest has already recorded the call phase as passed. A refusal there arrives as a separate ERROR on the same node-id and the test's own outcome stays passed. Anything counting passes — --pgc-expect-tests, a CI summary, a human reading "N passed" — sees a pass.

Nothing was broken

This layer's vacuity guard is already in the call-phase wrapper, which is why a test that concludes nothing is failed rather than passed-with-an-error. What was missing is that nothing said so.

Moving it into the expect fixture's teardown is a plausible-looking refactor — the fixture is right there, it already has a teardown, and a guard reads as cleanup — and it would have turned every vacuous test in the corpus from failed into passed with an error beside it. The corpus would still have exited non-zero, so it is not silent; but the per-test verdict, which is what --pgc-expect-tests and every summary read, would have said pass.

Two arms, and the second is the control

Without the control the first arm passes whatever phase the guard is in: "a vacuous test fails" is equally true of a correctly placed guard and of no guard at all standing next to an unrelated failure. So the second arm asserts that the teardown shape really does report a pass, which states the mode as a measurement rather than as a warning.

Proved by moving it: with the call-phase raise disabled and the same refusal placed after the expect fixture's yield, the first arm reddens. Restored, 11 passed.

Narrowed, not closed

VACUITY_MODES.md 3.7 now says which half is which.

  • Closed: this layer's own guard placement, pinned with a control.
  • Still open: the general shape. A guard anyone adds later in a teardown still cannot fail its test, and nothing refuses that.

I would rather leave that written down than claim the family. The inventory's counts are therefore unchanged — the mode stays in section 3, which is why none of the totals move in this diff.

Evidence

pytest corpus       164 passed
harness_selftest    538 checks, 538 passed + 0 failed + 0 unrunnable, rc 0
the new arms        2, and the first reddens when the guard moves to the teardown

Relationship to my other open PRs

Independent, off main at f0f1f40, like #926, #927 and #930 — none blocks another. This one touches test_runshape.py, TESTS.md section 10, VACUITY_MODES.md 3.7 and CHANGELOG.md.

It will conflict with #927 and #930 in VACUITY_MODES.md and CHANGELOG.md — the append-at-the-top kind, not a code conflict. Those two also move a mode from section 3 to section 2 and so edit the count rows; this one does not, because 3.7 is narrowed rather than closed. Whichever lands last, I will resolve and re-run both harnesses rather than adjust numbers by hand.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a

…mmandprompt#432)

Measured, the same AssertionError raised from two places:

    from a pytest_runtest_call wrapper   1 failed
    from a fixture teardown              1 passed, 1 error

pytest has already recorded the call phase as passed by the time a teardown runs, so a
refusal there arrives as a separate ERROR on the same node-id and the test's own outcome
stays `passed`. Anything counting passes -- --pgc-expect-tests, a CI summary, a human
reading "N passed" -- sees a pass.

NOTHING WAS BROKEN. This layer's vacuity guard is already in the call-phase wrapper,
which is why a test that concludes nothing is FAILED rather than passed-with-an-error.
What was missing is that nothing said so. Moving it into the `expect` fixture's teardown
is a plausible-looking refactor -- the fixture is right there, it already has a teardown,
and the guard reads as cleanup -- and it would have turned every vacuous test from
`failed` into `passed` with an error beside it.

TWO ARMS, AND THE SECOND IS THE CONTROL. Without it the first passes whatever phase the
guard is in: "a vacuous test fails" is equally true of a correctly placed guard and of no
guard at all standing next to an unrelated failure. So the second arm asserts the teardown
shape really does report a pass, which states the mode as a measurement rather than as a
warning.

PROVED BY MOVING IT. With the call-phase raise disabled and the same refusal placed after
the `expect` fixture's yield, the first arm reddens; restored, 11 passed.

NARROWED, NOT CLOSED, and VACUITY_MODES.md 3.7 says which half. Closed: this layer's own
guard placement. Still open: the general shape -- a guard anyone adds later in a teardown
still cannot fail its test, and nothing refuses that. I would rather leave that written
down than claim the family.

MEASURED
  pytest corpus       164 passed
  harness_selftest    538 checks, 538 passed + 0 failed + 0 unrunnable, rc 0
  the new arms        2, and the first reddens when the guard moves to the teardown

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 3d87b4c0, 12/12 SUCCESS. I verified the claim, the removal proof, and the control separately, because the control is what makes the first arm worth anything and you say so yourself.

The claim reproduces, independently of your fixtures. Two files of my own, the same AssertionError from two places:

call phase  ->  FAILED test_call.py::test_x       ->  1 failed
teardown    ->  ERROR  test_teardown.py::test_x   ->  1 passed, 1 error

And the thing that matters, what a pass-counter reads off the teardown run:

1 passed
1 error

So --pgc-expect-tests and every summary would see a pass. The table in your body is exact.

The removal proof holds. I moved the guard the way the refactor would — deleted the VacuityError from the pytest_runtest_call wrapper and raised the identical error after the expect fixture's yield:

FAILED test_runshape.py::test_the_vacuity_guard_fails_the_test_rather_than_erroring_beside_it
1 failed, 10 passed

Exactly the arm that claims the guard fails the test, and nothing else. Baseline 11 passed; pgc_vacuity.py restored byte-identical.

The control is a real assertion, not a shape. expect.outcomes(result, ..., passed=1, failed=0, errors=1) reddens on each field independently — I mutated each of the three in turn and got 1 failed every time. So it pins the outcome triple rather than merely observing that something happened. It also cannot be reddened by any change to this layer, which is correct for what it is: it pins an assumption about pytest, and if pytest ever stopped reporting a teardown failure this way, the placement argument would need revisiting and this is the arm that would say so.

It still composes. #927 landed on main while I was reviewing this, so I merged your branch onto the new main locally: clean, 4 files changed, 121 insertions(+), 2 deletions(-), no conflict.

On the scoping, which is the part I would have argued about if you had not. "Nothing was broken — what was missing is that nothing said so" is the honest framing, and narrowing 3.7 rather than claiming the family is the right call. A guard anyone adds later in a teardown still cannot fail its test, the totals correctly do not move, and the mode stays in section 3. That is a measurement with its limits stated, which is worth more than a closed row that is not really closed.

Nothing blocking, nothing I could break.

@jdatcmd
jdatcmd merged commit aa53c1b into commandprompt:main Sep 10, 2026
12 checks passed
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Sep 10, 2026
One conflict, CHANGELOG.md: both sides append a bullet at the top of the same
section. Resolved by keeping BOTH, since choosing one loses a shipped entry.
TESTS.md and VACUITY_MODES.md auto-merged -- commandprompt#931 narrowed section 3.7 without
moving a mode, so the 27 / 45 / 72 counts this branch composed still hold.

The corpus on the merged tree: 205 passed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Sep 10, 2026
…080 sweep fix

One conflict, CHANGELOG.md, and both sides append a bullet at the top of the same
section -- kept both. commandprompt#927 deleted test/selftest/440 on main and this branch never
touched it, so nothing else met.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Sep 10, 2026
…harness-guards branch

Two conflicts, both additive. CHANGELOG.md: two regions, both sides appending at the
top of the same section -- kept both. TESTS.md: both sides number a section, and the
raises section main landed at 18 collides with this branch, so it is renumbered 20
and its TOC entry and anchor follow.

Verified structurally: 20 headings, 20 TOC entries, numbers contiguous 1..20, titles
identical between the two lists, every TOC anchor equal to the anchor GitHub derives
from its heading.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
jdatcmd added a commit that referenced this pull request Sep 10, 2026
Two corrections from @OffgridwithJD's review, and the first one is the
document's own rule 3 catching the document.

"39 call sites" was 36 calls plus the 3 definitions. The pattern `[^_a-z]_sh(`
matches `def _sh(` as readily as a call, which is the same class of error as
`[a-z_]+\.sh` matching `sharedir` -- already written three lines above as the
thing not to do. Counted with ast now, and the entry says how, because a number
in this section has to be re-derivable or it does not belong here.

The heading said 4 python files. Three are on main; the fourth arrives with
PR #923. The entry always said so, the heading did not, and a reader who stops
at the bold line gets a count that is wrong today.

Recounted against main at aa53c1b, after #927 and #931 landed: still 3 python
files and 7 shell files. test_raises_sqlstate.py, new on main, adds neither --
it drives pytester, not the shell.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
jdatcmd added a commit that referenced this pull request Sep 10, 2026
#927, #931 and #930 landed while this waited on review, so main gained three
CHANGELOG entries and two TESTS.md sections.

CHANGELOG: both sides append at the top of the same section and neither
replaces anything, so the union is the resolution.

TESTS.md: the numbering collided. This branch inserted its file section at 15
and pushed "Adding a test", "What this corpus does NOT yet refuse" and "Traps
this corpus records" to 16-18; main kept those at 15-17 and appended its two
new file sections as 18 and 19. Auto-merge produced two sections numbered 18.

Resolved main's way, because main's convention is now to append a new file
section after the tail sections: this branch's section becomes 20, and the
three tail sections go back to main's 15, 16 and 17. That renumbers one
section of this branch rather than two of main's.

Checked rather than eyeballed, because an anchor that stops resolving does not
announce itself: 20 headings against 20 TOC entries, every TOC text equal to
its heading, every anchor equal to what GitHub derives from that heading, and
the numbering contiguous 1..20.

On the composed tree: selftest 350 41 checks 0 failed, selftest 400 64 checks
0 failed, selftest 080 15 checks 0 failed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
jdatcmd added a commit that referenced this pull request Sep 10, 2026
#923 moved five times while this waited, and #921, #927, #930, #931 and #932
landed on main underneath it. Composing found two things that a clean merge
would not have.

TESTS.md: the base now carries 22 sections, and this branch had inserted the
ledger at 16. Auto-merge kept the base's tail sections AND this branch's copies
of them, so the file would have had two of each. Resolved by keeping only the
ledger section from this side, renumbered to 23, where the base's own 22 already
ends. Checked rather than eyeballed: 23 headings against 23 TOC entries, every
TOC title equal to its heading, every anchor equal to GitHub's derivation,
numbering contiguous 1..23, no duplicate heading.

NO_CLUSTER gains test_mutation_ledger.py. #921's classifier arrived on the base
and immediately named it:

    membership_report: [1: undeclared:test_mutation_ledger.py]

It drives test/pgc_ledger.py, a python tool rather than the shell harness, so it
needs neither a cluster nor psycopg. Measured rather than assumed: 9 passed in a
venv with no driver, and driver_dependent() agrees. The derived job goes to 11
files and 179 passed.

THE LEDGER IS REGENERATED, AND THAT IS THE POINT OF THIS MERGE RATHER THAN A
SIDE EFFECT. #923 added 17 checks to selftest 400 and converted 25 skip sites,
none of which the committed ledger had ever seen. The gate refuses a check it
has never seen, so the composed tree would have failed CI for a reason with
nothing to do with either change. Regenerating is the documented repair, and the
budget file says so.

From a real run of the composed tree, not a synthesised log:

    harness_selftest.sh: PASSED, rc=0
    checks run: 735 | accounting: 735 passed + 0 failed + 0 unrunnable + 0 skipped
    ledger: 701 rows -> 734 | never=734, ever red=0
    gate: new this run=0

Reconciled: 735 records == 732 distinct (suite, part, name) + 3 names that each
appear twice in one run, and 0 log triples are missing from the ledger, which is
exactly what the gate refuses. All 734 rows carry five fields and none ends in a
tab. The budget's asserted census follows to 734.

Two ledger rows do not appear in this log -- selftest 330's "all three runner
functions", which #923 changed to five. rename-scan reports appeared=0,
vanished=2. THEY ARE LEFT DELIBERATELY: this log is PG17 only, and pruning rows
that a single major did not produce would delete checks that legitimately run
elsewhere. The gate refuses unseen checks, not unused rows.

Gates on the composed tree: 350 53/53, 400 81/81, 410 96 checks 0 failed,
080 15/15, shellcheck rc=0 over the whole harness, driver-free job 11 files
179 passed, membership_report [].

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
jdatcmd pushed a commit that referenced this pull request Sep 10, 2026
`INSERT ... SELECT ... WHERE false` writes nothing and raises nothing. psycopg
reports `INSERT 0 0` with a `rowcount` of 0, and nothing in the corpus read either
field -- so the fixture a test meant to build did not exist, and every assertion
below it compared two empty things. That is `insert-wrote-no-rows`, entry 2 on
VACUITY_MODES.md's own list of what to add next.

Measured before building: 12 write statements across 5 files, and `rowcount` read
at exactly one site, which does not assert on it.

THE COMMAND TAG DECIDES, NOT THE ROW COUNT. `SELECT 0` and `INSERT 0 0` both carry
`rowcount == 0`, so a guard keyed on the count alone would refuse every test whose
last statement was a SELECT over an empty result -- a legitimate and common
assertion. `statusmessage` is the server's own command tag, so this guard never
parses SQL. Measured on PG 18 against a pgcolumnar table:

    statusmessage       rowcount   statement
    CREATE TABLE              -1   CREATE TABLE t (i int) USING pgcolumnar
    INSERT 0 5                 5   INSERT INTO t SELECT g FROM generate_series(1,5) g
    INSERT 0 0                 0   INSERT ... WHERE false
    UPDATE 0                   0   UPDATE t SET i = i WHERE i > 100
    DELETE 0                   0   DELETE FROM t WHERE i > 100
    SELECT 0                   0   SELECT * FROM t WHERE false
    SET                       -1   SET search_path TO public
    TRUNCATE TABLE            -1   TRUNCATE t

A DELIBERATE ZERO STAYS WRITABLE. A DELETE that must match nothing is a real
negative control, so `expect.wrote(cur, 0, name)` both compares the count and marks
the write as named. An unnamed zero fails the test; naming a count does not excuse
a wrong one; and a `rowcount` of -1 is refused rather than compared, for the reason
`expect.rowcount` already records.

IN THE CALL PHASE, NOT A TEARDOWN. #931 measured that a guard run as a teardown
fixture reports the test it guards as PASSED and fails separately, so a reader sees
a green test beside an error.

TWO PROPERTIES, TWO FILES, AND THE SPLIT IS MEASURED RATHER THAN ASSERTED. The
classifier and the refusal live in test_writes_wrote_rows.py, which needs no
database: a stub cursor carrying the two measured fields exercises them exactly.
Whether the connection the tests actually use is watched is a different claim that
no driver-free arm can make, and test_the_connection_the_tests_use_is_watched makes
it through a real `INSERT ... WHERE false`, on both `conn.execute` and a cursor the
connection handed out -- 24 and 42 sites in the corpus, so a proxy watching only the
connection would leave most of it unwatched.

Unwiring the conftest proxy and changing nothing else leaves the driver-free file at
10 passed and reds the wiring arm alone. That is #917's defect in miniature: its
pytest twin tested the reconciler's body and left the runner's CALL to it
uncovered, so removing the call kept the pytest half green at 9 passed while the
shell half went red by one.

PROVE BY REMOVAL, five mutations, each applied by exact string match with the file
asserted to still parse:

    control                       free 10 passed    wiring 9 passed
    no refusal                    free  2 failed    wiring 9 passed
    command tag ignored           free  2 failed    wiring 1 failed
    every statement is a write    free  4 failed    wiring 1 failed
    connection not wrapped        free 10 PASSED    wiring 1 failed
    acknowledgement not recorded  free  1 failed    wiring 1 failed

FALSE-POSITIVE BUDGET FIRST, because a guard that reddens a legitimate write is
worse than the mode: the full corpus with a cluster is 249 passed, and none of the
12 write sites already in the corpus reddened.

TWO OF MY OWN ARMS WERE WRONG, recorded because each produced a green that meant
nothing:

  * An arm asserting only `failed=1` passed before the feature existed. The inner
    test called a function not yet written, got an AttributeError, and the outer arm
    read that as the comparison failing. Naming the numbers is what makes the red
    the right red.
  * A multi-word pattern can straddle pytest's word wrap. `expect.refusal` anchors
    each pattern to one `E` line and pytest wraps a long traceback line, so matching
    "wrote no rows" failed against a message containing it -- which reads exactly
    like "the guard did not fire". The refusal now leads with the mode's own
    kebab-case id, which is one token and cannot be split.

Docs carried in the same commit, per jd's rule. VACUITY_MODES.md moves the mode to
section 2 and keeps a back-reference where it was, and its stated totals go 27 -> 28
refused and 45 -> 44 not refused -- checked by the corpus's own arms, which is how I
learned the numbers rather than deciding them. TESTS.md gains section 22, and the
paragraph in section 18 that listed this gap is rewritten rather than deleted,
because a reader who knew the gap needs to find out where it went.

Gate on this tree:
    harness_selftest   588 checks, 588 passed + 0 failed + 0 unrunnable, PASSED
    driver-free job    10 files, 171 passed, psycopg absent from the venv
    full corpus        249 passed with a cluster on pg18a

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants